Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose the Lock ID on the locks page #127

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

stuartskelton
Copy link
Contributor

Summary

Expose the lock ID on the Minion admin locks page

Motivation

I employ locks to manage job execution across multiple worker boxes. However, apart from querying the database, I'm unable to determine whether the locks are being rotated. The expiration time decreasing provides some indication, but if the jobs finish within a minute, I can't tell solely from the lock screen whether they are cycling or being held for that minute.

Since it's only retrieving the ID, which would be part of the index, there should be no slowdown in the database.

References

Sorry this is a drive by pull request

@kraih kraih requested review from a team, marcusramberg, kraih, jhthorsen and jberger March 14, 2024 13:36
Copy link
Member

@kraih kraih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not been in a situation where i was interested in the lock id personally. But the reasoning makes sense.

kraih
kraih previously requested changes Mar 14, 2024
Copy link
Member

@kraih kraih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, there is documentation changes missing. And adding a small test to pg_admin.t also wouldn't be hard.

@mergify mergify bot dismissed kraih’s stale review March 19, 2024 11:51

Pull request has been modified.

@stuartskelton stuartskelton force-pushed the expose_lock_id_on_locks_page branch from ee541da to 4f7873f Compare March 19, 2024 11:52
@stuartskelton
Copy link
Contributor Author

Thanks for the pointers.

I have added the changes and added the test.

I also took the time to add some tests for the status endpoint for the locks too.

kraih
kraih previously requested changes Mar 19, 2024
Changes Outdated
@@ -1,5 +1,6 @@

10.29 2023-11-28
- Improved admin ui to show the lock ID on the locks page.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't write your own Changes entries please, those are done by the maintainers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead it needs to be mentioned in the documentation, like the other fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are correct, I have corrected this.

@kraih
Copy link
Member

kraih commented Mar 19, 2024

And don't forget to squash your commits when you are ready for review.

@stuartskelton stuartskelton force-pushed the expose_lock_id_on_locks_page branch from 4f7873f to 01bab6f Compare March 19, 2024 13:56
@mergify mergify bot dismissed kraih’s stale review March 19, 2024 13:57

Pull request has been modified.

@stuartskelton stuartskelton force-pushed the expose_lock_id_on_locks_page branch from 01bab6f to 183b7ba Compare March 19, 2024 14:16
kraih
kraih previously requested changes Mar 19, 2024
id => 1

Lock id.

Copy link
Member

@kraih kraih Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple places where this needs to be added. Also the fields are in alphabetical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated Backend.pm, I think this was the only other place I neede

@kraih
Copy link
Member

kraih commented Mar 19, 2024

Also a perltidy fix is needed. There will be a new Minion release in a few hours, if the mentioned points are resolved this PR can be included.

@stuartskelton stuartskelton force-pushed the expose_lock_id_on_locks_page branch from 183b7ba to d1489cf Compare March 19, 2024 15:56
@stuartskelton stuartskelton force-pushed the expose_lock_id_on_locks_page branch from d1489cf to 90cbf56 Compare March 19, 2024 15:56
@mergify mergify bot dismissed kraih’s stale review March 19, 2024 15:56

Pull request has been modified.

@stuartskelton
Copy link
Contributor Author

ok,

PT'ed the test.
Added the docs to the Backend.pm
And I found the Jobs command that spits out the lock data. So I updated that too.

@kraih kraih merged commit 0d302ac into mojolicious:main Mar 19, 2024
7 checks passed
@stuartskelton stuartskelton deleted the expose_lock_id_on_locks_page branch March 19, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants